Run clang-format in the test suite rather than as a separate CI step#218
Merged
Conversation
This change simplifies the CI configuration and makes it somewhat easier to verify code formatting locally. Running the tests with ctest will run the format check after all the other tests have completed. The format check is on by default unless clang-format cannot be found on the user's system. It can be turned off by passing -DTT_FORMAT_CHECK=0 to the CMake configuration step. Any new tests must currently be added manually to the FORMAT_TARGETS list. The only consequence of failing to update the list is that the formatting check will not be run on the necessary test sources. The libtopotoolbox sources will still be checked. I have also added internal header files to the various library/executable targets. This seems not to break anything and is necessary to get them checked as well. Signed-off-by: William Kearney <william.kearney@uni-potsdam.de>
The snapshot tests won't run on Windows or macOS because they don't have GDAL, but this prevents them trying to download the snapshot data anyway.
Contributor
|
@wkearn your new approach seems simpler! I prefer the new everything-in-one version, we don't write tests that often so it should not be a problem |
Member
Author
|
I will merge this then. Let me know if it does prove to be burdensome or if it is more flaky or unreliable this way. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change simplifies the CI configuration and makes it somewhat easier to verify code formatting locally. Running the tests with ctest will run the format check after all the other tests have completed.
The format check is on by default unless clang-format cannot be found on the user's system. It can be turned off by passing -DTT_FORMAT_CHECK=0 to the CMake configuration step.
Any new tests must currently be added manually to the FORMAT_TARGETS list. The only consequence of failing to update the list is that the formatting check will not be run on the necessary test sources. The libtopotoolbox sources will still be checked.
I have also added internal header files to the various library/executable targets. This seems not to break anything and is necessary to get them checked as well.
This required some fiddling with the CI configuration to ensure that if the snapshot tests are not built because GDAL is missing then the clang-format configuration step doesn't try to look at the nonexistent snapshot target.
@bgailleton: Do you prefer having a separate CI job for the formatting checks? I am hoping that this makes it easier to get the formatting right locally because you don't have to remember the exact clang-format incantation, but it might hide formatting errors in the test output.